Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

MQE: fix issues with vector/vector binary comparsion operations #10235

Draft
wants to merge 43 commits into
base: main
Choose a base branch
from
Draft

MQE: fix issues with vector/vector binary comparsion operations #10235

wants to merge 43 commits into from

Conversation

charleskorn
Copy link
Contributor

@charleskorn charleskorn commented Dec 13, 2024 •
edited
Loading

What this PR does

This PR fixes two issues with the implementation of comparison operations between two vectors without the bool modifier in MQE:

  • Queries would return incorrect results if the left side contained series with different metric names, and all results would be merged into a single series rather than returned in separate series for each metric name
  • Queries would incorrectly fail with a conflict error if multiple series on the left side matched the same series on the right side, those left side series had points at the same timestamp, and one or none of those points remained after applying the comparison operation

This PR builds on #10119 and must be merged after #10119 is merged.

This PR is a draft pending benchmarking and profiling of the impact of these changes.

Which issue(s) this PR fixes or relates to

#10067

Checklist

  • Tests updated.
  • [n/a] Documentation added.
  • [covered by Mimir Query Engine #10067] CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • [n/a] about-versioning.md updated with experimental features.

@charleskorn
Add feature toggle
dc874aa 
# Conflicts:
#	cmd/mimir/config-descriptor.json
#	cmd/mimir/help-all.txt.tmpl
#	docs/sources/mimir/configure/configuration-parameters/index.md
#	pkg/streamingpromql/config.go
#	pkg/streamingpromql/engine_test.go
@charleskorn
Enable upstream test cases
d9a467e
@charleskorn
Remove comments about and/or/unless.
aad0179
@charleskorn
Add some test cases
37434ad
@charleskorn
Make condition clearer
31c886f
@charleskorn
Add comparison operation edge cases
6ec6e0e
@charleskorn
Add tests for comparison operators with group_left / group_right
fa12925
@charleskorn
Update tests to reflect prometheus/prometheus#15471
a09c1ac
@charleskorn
Expand test to confirm label handling behaviour
4965398
@charleskorn
Rename existing operator
64114f7
@charleskorn
Add structure for new operator
749789b
@charleskorn
Initial implementation of SeriesMetadata
9ce7407
@charleskorn
Initial implementation
882b2a5
@charleskorn
Handle case where one set of many-side series many to many output series
0ea8687
@charleskorn
Use correct labels when grouping one side series
b29c99e
@charleskorn
Return a conflict error message if there are multiple samples at the …
11bf6b0 
…same timestamp on the same "one" side for the same group with different additional labels
@charleskorn
Fix handling of cases where additional labels appear on the "many" side
d48be6a
@charleskorn
Return a non-misleading error message when a conflict occurs on the "…
4e1f6fa 
…many" side
@charleskorn
Update comments
1876813
@charleskorn
Fix regression in comparison operation output labels
d35f355
@charleskorn
Disable one-to-one comparison operation cases that fail for known rea…
87bbd46 
…sons
@charleskorn
Fix linting warnings and simplify computeOutputSeries()
a53580a
@charleskorn
Add tests for annotations
331cf70
@charleskorn
Add tests for case where additional labels are not present on series …
606a96d 
…on either side
@charleskorn
Add series sorting test
8c56b2d
@charleskorn
Add provenance comment
25be40f
@charleskorn
Add benchmark
502245f
@charleskorn
Expand comments
0b8fa34
@charleskorn
Fix typo in test names
7e4113a
@charleskorn
Add test cases with label names in different orders
c41b575
@charleskorn
Add some test cases with native histograms
230adfe
@charleskorn
Ensure buffers passed to labels.Labels.Bytes(), BytesWithLabels() and…
a6d8763 
… BytesWithoutLabels() are reused if resized
@charleskorn
Address PR feedback: use minimal number of points for binary operatio…
e467023 
…n slice
@charleskorn
Address PR feedback: rename latestSeries to latestSeriesIndex
aabd093
@charleskorn
Address PR feedback: add docstring for updatePresence
8c8f782
@charleskorn
Address PR feedback: try to reuse slices in more cases
7aa29ee
@charleskorn
Run mixed metrics tests in parallel
153f66f
@charleskorn
Add group_left to mixed metrics tests
3a262bf
@charleskorn
Address PR feedback: refactor `vectorVectorBinaryOperationEvaluator.c…
d2e2a1c 
…omputeResult` to reduce nesting
@charleskorn
Add early filtering test case for group_right
2f22459
@charleskorn
Fix issue where comparison operations without the bool modifier would…
f8b79e8 
… return incorrect results if the left side contained series with different metric names
@charleskorn
Avoid expensive labels.Labels.String() call
cad9fa0
@charleskorn
Fix issue where comparison operations between two vectors incorrectly…
d8b53a1 
… fail with a conflict if multiple left series match the same right series and only one left point remains after applying the comparison
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tacole02 tacole02 Awaiting requested review from tacole02 tacole02 will be requested when the pull request is marked ready for review tacole02 is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@charleskorn